New Linter rule: MisleadingCondition#2184
Conversation
tapple
changed the title
tapple
changed the title
|
this should be documented on a rfc and approved first before creating a new linter rule i think |
Contribute suggests rfcs only for language changes, this wouldn't do that, and an issue has been created, though maybe some more time should have passed between issue and pr. On the rule in general, I think it's extremely useful for devs unfamiliar with lua. Nearly every other language has at least 0 as false, and many have a concept of falsey for things like empty strings or tables. As lua and by extension luau doesn't have a broad falsey concept, this lint would catch a very common footgun for newer users. |
| @@ -19,7 +19,7 @@ TEST_CASE_FIXTURE(Fixture, "CleanCode") | |||
| { | |||
There was a problem hiding this comment.
I believe the 2 remaining unit test failures are due to a typechecker bug:
|
Can't comment on all the cases, but I like the idea. I think this is also valuable for a case where you have a function |
tapple
changed the title
The linter is not part of the language itself, and frankly will probably be sunset in favor of the linter we're writing in lute since that allows programmers to author their own lints in luau instead of writing C++ to lint luau. Even if it continues to exist as well, it's not something we've treated as requiring an RFC. |
| { | ||
| const char * msg; | ||
| bool negated; | ||
| if (!checkCondition(node->condition, &msg, &negated)) |
There was a problem hiding this comment.
TODO: also check the first argument to assert()
There was a problem hiding this comment.
Or maybe don't. An assert evaluating false is usually understood to be a bug. And the typechecker should often be able to catch the same bugs (at leasts the asserts that assert not nil). So nearly every time this linter rule flagged an assert, it would be a false warning.
also, asserts are pretty specialized. this linter rule is meant to catch common, stupid errors. And I don't think the linter even could distinguish between "asserts that meant to check a boolean but forgot a comparison operator" and "asserts that are checking for non-nil and the type-checker agrees". Maybe checking for intersection types, but either way, it's different enough that it should be a different linter rule
There was a problem hiding this comment.
An assert evaluating false is usually understood to be a bug.
I don't think this is true at all. We use things like assert(false) or assert("this message to read about an unreachable assumption" && false) all the time to mark branches that we expect to be unreachable.
| if (isBoolean(*type)) | ||
| return true; // boolean is a valid condition | ||
| if (isOptional(*type)) | ||
| return true; // anything that can be nil is a valid condition |
There was a problem hiding this comment.
using this on my own code, I've only found one pattern so far where I can't rewrite the code to shut this linter rule up and the linter rule seems wrong:
local list: {number} = {1, 2, 3}
local n5 = list[5] or 5 -- Misleading Condition
direct indexing can return nil if the list is too short, and the typechecker can't detect that
I think I should handle this case specifically
5 participants
The linter now complains about condition expressions that are not one of:
booleannilThe linter considers a condition expression to be one of:
ifstatementifexpressionandoperatororoperatorSee the unit test for error messages